Add documentation for Process.Kill(bool entireProcessTree) #2828

wtgodbe · 2019-07-25T20:21:48Z

Adds documentation for method System.Diagnostics.Process.Kill(bool entireProcessTree)

CC @krwq for area co-ownership, @bgribaudo who added this method, @carlossanlop for CoreFx documentation effort ownership, and @rpetrusha @mairaw for grammar concerns.

mairaw

LGTM. Here's what I've done:
Since the remarks were exactly the same as the other overload, I've moved them to be higher on the page and applicable to the whole page, not just the individual overloads. Let me know what you think!

carlossanlop · 2019-07-26T00:38:34Z

I think it looks good. If @wtgodbe aggrees with the additional commits, then maybe we can get it merged.

mairaw · 2019-07-26T01:01:31Z

The reflection tool will undo your last change @carlossanlop from what I can tell 😉

mairaw · 2019-07-26T01:02:54Z

@wtgodbe you can see how the changes would look like here: https://review.docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.kill?view=netcore-3.0&branch=pr-en-us-2828

rpetrusha

I've left a couple of suggestions and a comment, @wtgodbe.

rpetrusha · 2019-07-25T23:14:50Z

xml/System.Diagnostics/Process.xml

-        <summary>To be added.</summary>
-        <remarks>To be added.</remarks>
+        <param name="entireProcessTree">
+          <see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only associated process.</param>


Suggested change

<see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only associated process.</param>

<see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only the associated process.</param>

rpetrusha · 2019-07-26T06:49:55Z

xml/System.Diagnostics/Process.xml

+The `Kill` method forces a termination of the process, while <xref:System.Diagnostics.Process.CloseMainWindow%2A> only requests a termination. 
+When a process with a graphical interface is executing, its message loop is in a wait state. 
+The message loop executes every time a Windows message is sent to the process by the operating system. 
+Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close to the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application. 


Suggested change

Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close to the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application.

Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application.

rpetrusha · 2019-07-26T06:54:04Z

xml/System.Diagnostics/Process.xml

+
+ -or-  
+
+ The associated process is a Win16 executable.</exception>


Aren't this and line 2136 legacy conditions -- perhaps they should be deleted? (Are there still Win16 executables running on .NET Framework?)

rpetrusha · 2019-07-26T06:56:17Z

xml/System.Diagnostics/Process.xml

+Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close to the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application. 
+The request to exit the process by calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> does not force the application to quit.
+The application can ask for user verification before quitting, or it can refuse to quit. 
+To force the application to quit, use the `Kill` method.


Suggested change

To force the application to quit, use the `Kill` method.

To force the application to quit, use the `Kill` method.

bgribaudo · 2019-07-26T14:08:21Z

Thanks for working on this, @wtgodbe! I added a couple change ideas as comments on code.

carlossanlop · 2019-07-26T16:04:33Z

@bgribaudo do you mean you backported the comments added here into the source code, or did you mean to add suggestions to this PR?

wtgodbe · 2019-07-26T16:11:25Z

Thanks for the feedback @rpetrusha, I've pushed a commit to address it. @bgribaudo I couldn't find your comments, could you point me towards them?

bgribaudo · 2019-07-26T17:06:41Z

@carlossanlop:

@bgribaudo do you mean you backported the comments added here into the source code, or did you mean to add suggestions to this PR?

Oops! I meant as comments on this change's Markdown code diff. Sorry for the confunsion. :-(

bgribaudo · 2019-07-26T13:48:29Z

xml/System.Diagnostics/Process.xml

+
+> [!NOTE]
+> The <xref:System.Diagnostics.Process.Kill%2A> method executes asynchronously. 
+> After calling the `Kill` method, call the <xref:System.Diagnostics.Process.WaitForExit%2A> method to wait for the process to exit, or check the <xref:System.Diagnostics.Process.HasExited%2A> property to determine if the process has exited.


To make very clear that these ways of checking exit status only apply to the current process instance, maybe add something along the lines of the following?

"Note: WaitForExit() and HasExited do not reflect the status of descendant processes. When Kill(entireProcessTree: true) is used, WaitForExit() and HasExited will indicate that exiting has completed after the given process exits, even if all descendants have not yet exited."

bgribaudo · 2019-07-26T14:05:37Z

xml/System.Diagnostics/Process.xml

-        <param name="entireProcessTree">To be added.</param>
-        <summary>To be added.</summary>
+        <param name="entireProcessTree"><see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only the associated process.</param>
+        <summary>Immediately stops the associated process, and optionally all of its child/descendent processes.</summary>
        <remarks>To be added.</remarks>


Suggestion: Drop the word "all" above (from "optionally all of its child...") and note here that, when set to true, processes where the call lacks permissions to view details will be silently skipped by the descendant termination process because the termination process is unable to determine whether those processes are descendants.

bgribaudo · 2019-07-26T17:11:18Z

@wtgodbe:

@bgribaudo I couldn't find your comments, could you point me towards them?

Maybe they didn't show up because I hadn't submitted the review (didn't realize that might be necessary :-(). Can you see them now?

mairaw · 2019-07-26T17:13:52Z

Now they appear @bgribaudo!

wtgodbe · 2019-07-26T20:29:35Z

@bgribaudo @carlossanlop @rpetrusha @mairaw PTAL at 61b1a6d

rpetrusha · 2019-07-26T21:17:42Z

The build failed. I've corrected the error, and a new build has started.

carlossanlop · 2019-07-29T16:25:36Z

@rpetrusha thanks for correcting the error, the build passed. This is fully baked and ready to get merged.

rpetrusha · 2019-07-29T18:33:41Z

Thanks for the additional changes, @wtgodbe. I'll merge your PR now.

bgribaudo · 2019-08-07T13:26:19Z

Thank you, @wtgodbe and team!

Add documentation for Process.Kill(bool entireProcessTree)

ab45e8f

mairaw assigned wtgodbe Jul 25, 2019

mairaw added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Jul 25, 2019

mairaw added this to the July 2019 milestone Jul 25, 2019

Update Process.xml

d063abf

mairaw approved these changes Jul 25, 2019

View reviewed changes

mairaw added verify-build-before-merge waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged labels Jul 25, 2019

carlossanlop requested review from krwq and carlossanlop July 26, 2019 00:35

minor fix: parm summary in single line

f4e7421

carlossanlop approved these changes Jul 26, 2019

View reviewed changes

rpetrusha suggested changes Jul 26, 2019

View reviewed changes

Address feedback

9b8e826

bgribaudo reviewed Jul 26, 2019

View reviewed changes

Address Feedback

61b1a6d

Fixed bad xml element

0a2e347

rpetrusha closed this Jul 26, 2019

rpetrusha reopened this Jul 26, 2019

carlossanlop removed the waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged label Jul 29, 2019

rpetrusha approved these changes Jul 29, 2019

View reviewed changes

rpetrusha merged commit aafba2c into dotnet:master Jul 29, 2019

mairaw removed the verify-build-before-merge label Aug 6, 2019

	<see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only associated process.</param>
	<see langword="true" /> to kill the associated process and its descendants; <see langword="false" /> to kill only the associated process.</param>

	Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close to the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application.
	Calling <xref:System.Diagnostics.Process.CloseMainWindow%2A> sends a request to close the main window, which, in a well-formed application, closes child windows and revokes all running message loops for the application.


		-or-

		The associated process is a Win16 executable.</exception>

	To force the application to quit, use the `Kill` method.
	To force the application to quit, use the `Kill` method.

Add documentation for Process.Kill(bool entireProcessTree) #2828

Add documentation for Process.Kill(bool entireProcessTree) #2828

Uh oh!

Conversation

wtgodbe commented Jul 25, 2019

Uh oh!

mairaw left a comment

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Jul 26, 2019

Uh oh!

mairaw commented Jul 26, 2019

Uh oh!

mairaw commented Jul 26, 2019

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

rpetrusha Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

bgribaudo commented Jul 26, 2019

Uh oh!

carlossanlop commented Jul 26, 2019

Uh oh!

wtgodbe commented Jul 26, 2019

Uh oh!

bgribaudo commented Jul 26, 2019

Uh oh!

bgribaudo Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

bgribaudo Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

bgribaudo commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mairaw commented Jul 26, 2019

Uh oh!

wtgodbe commented Jul 26, 2019

Uh oh!

rpetrusha commented Jul 26, 2019

Uh oh!

carlossanlop commented Jul 29, 2019

Uh oh!

rpetrusha commented Jul 29, 2019

Uh oh!

bgribaudo commented Aug 7, 2019

Uh oh!

Uh oh!

bgribaudo commented Jul 26, 2019 •

edited

Loading